Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: fahrenheit to celsius in codebase, part 1 #2570

Merged
merged 11 commits into from
Sep 21, 2023
Merged

refactor: fahrenheit to celsius in codebase, part 1 #2570

merged 11 commits into from
Sep 21, 2023
+134 −153

Conversation

scarf005
Copy link
Member

@scarf005 scarf005 commented Apr 5, 2023 •
edited
Loading

Summary

SUMMARY: Infrastructure "Replace fahrenheit usage to celsius in codebase, part 1"

Purpose of change

Describe the solution

  • used units::temperatures for temperatures constants
  • used units::temperatures for item rotpoint calculation
  • hardcoded rot_chart as conversion from fahrenheit to celsius was tricky
  • also simplified if-else pyramid of doom in acid rain function

Describe alternatives you've considered

screm

Testing

all local test passed.

Additional context

help

@github-actions github-actions bot added the src changes related to source code. label Apr 5, 2023
github-actions[bot]
github-actions bot reviewed Apr 5, 2023
View reviewed changes
src/units_temperature.h Outdated Show resolved Hide resolved
@scarf005 scarf005 force-pushed the remove-fahrenheit branch from f76d5ca to 9845588 Compare April 5, 2023 03:00
@github-actions github-actions bot added the tests changes related to tests label Apr 5, 2023
github-actions[bot]
github-actions bot reviewed Apr 5, 2023
View reviewed changes
src/character.h Outdated Show resolved Hide resolved
@scarf005 scarf005 force-pushed the remove-fahrenheit branch 3 times, most recently from d572e2b to 819dcce Compare April 5, 2023 04:06
@TheEaterOfBeans
Copy link

ok but if temperature displays anywhere in game (I don't know if it does or not) let people choose to see it in Fahrenheit

@chaosvolt
Copy link
Member

ok but if temperature displays anywhere in game (I don't know if it does or not) let people choose to see it in Fahrenheit

Already a thing, if you have a thermometer it will show the ambient temp, and interface settings allow you to pick what unit it'll show as. PR's focus is on changing what unit it works with under the hood, which is then converted for display.

Coolthulhu
Coolthulhu reviewed Apr 5, 2023
View reviewed changes
src/character.cpp Outdated Show resolved Hide resolved
Coolthulhu
Coolthulhu reviewed Apr 5, 2023
View reviewed changes
src/suffer.cpp Outdated Show resolved Hide resolved
Coolthulhu
Coolthulhu reviewed Apr 5, 2023
View reviewed changes
src/suffer.cpp Outdated Show resolved Hide resolved
Coolthulhu
Coolthulhu reviewed Apr 5, 2023
View reviewed changes
tests/player_test.cpp Outdated Show resolved Hide resolved
Coolthulhu
Coolthulhu reviewed Apr 5, 2023
View reviewed changes
tests/behavior_test.cpp Outdated Show resolved Hide resolved
Coolthulhu
Coolthulhu reviewed Apr 5, 2023
View reviewed changes
src/gamemode_defense.cpp Outdated
@@ -98,7 +98,7 @@ defense_game::defense_game()
bool defense_game::init()
{
calendar::turn = calendar::turn_zero + 12_hours; // Start at noon
get_weather().temperature = 65;
get_weather().temperature = SPRING_TEMPERATURE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not caused here or anything like that, but nowadays our "effective spring temperature" is taken from json. In the future, this should be something like get_weather().recalculate().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you point out the part where weather_manager loads spring temperature from JSON? i couldn't find serialize or deserialize member function in weather_manager class.

scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 6, 2023
@scarf005 @Coolthulhu
refactor: explicitly use to_millidegree_celsius
5b3273f 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 6, 2023
@scarf005 @Coolthulhu
refactor: simplify celsisus to fahrenheit calls
64f7642 
discussed in
- cataclysmbnteam#2570 (comment)
- cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 6, 2023
@scarf005 @Coolthulhu
refactor: use milidegree_celsius
1cecb8b 
discussed in: cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 6, 2023
@scarf005 @Coolthulhu
refactor: make arbitrary temperatures look arbitrary
5d4c7e9 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
@scarf005 scarf005 marked this pull request as ready for review April 15, 2023 15:05
@scarf005 scarf005 changed the title refactor: fahrenheit to celsius in codebase refactor: fahrenheit to celsius in codebase, part 1 Apr 15, 2023
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 17, 2023
@scarf005 @Coolthulhu
refactor: explicitly use to_millidegree_celsius
ecabd31 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 17, 2023
@scarf005 @Coolthulhu
refactor: simplify celsisus to fahrenheit calls
2644dc6 
discussed in
- cataclysmbnteam#2570 (comment)
- cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 17, 2023
@scarf005 @Coolthulhu
refactor: use milidegree_celsius
056895f 
discussed in: cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
@scarf005 scarf005 force-pushed the remove-fahrenheit branch from 5d4c7e9 to 22add02 Compare April 17, 2023 15:14
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 17, 2023
@scarf005 @Coolthulhu
refactor: make arbitrary temperatures look arbitrary
22add02 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
@github-actions github-actions bot added the JSON related to game datas in JSON format. label Apr 17, 2023
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 17, 2023
@scarf005 @Coolthulhu
refactor: explicitly use to_millidegree_celsius
caff8e3 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Apr 17, 2023
@scarf005 @Coolthulhu
refactor: simplify celsisus to fahrenheit calls
eca33cf 
discussed in
- cataclysmbnteam#2570 (comment)
- cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
olanti-p
olanti-p suggested changes Sep 17, 2023
View reviewed changes
src/weather.cpp Outdated Show resolved Hide resolved
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Sep 18, 2023
@scarf005 @olanti-p
fix: make annual average namespaced
1541165 
see: cataclysmbnteam#2570 (comment)

Co-authored-by: olanti-p <[email protected]>
@scarf005 scarf005 requested a review from olanti-p September 18, 2023 00:26
scarf005 and others added 11 commits September 18, 2023 10:01
@scarf005
refactor: constexpr for TOOL_LIFT_FACTOR and JACK_LIMIT
1151c82
@scarf005
feat: add long double override for celsius literal
71b8241
@scarf005
refactor: use celsius on namespace temperatures
0ec5ba1
@scarf005
refactor: average_annual_termperature to units::temperature
ec0a63c
@scarf005
refactor: use _seconds notation
22cd6ab
@scarf005
refactor: use early return for weather_effect::acid
b74919e
@scarf005
refactor: calc_hourly_rotpoints_at_temp to accept units::temperature
d1d33ca 
please forgive me for shilling SI unit this much.

Hardcoded as changing to celsius reduces array size a lot,
and i've spent 6h unsuccessfully porting this calc_hourly_rotpoints_at_temp to work with celsius
@scarf005 @olanti-p
fix: make annual average namespaced
6ffe048 
see: cataclysmbnteam#2570 (comment)

Co-authored-by: olanti-p <[email protected]>
@scarf005
fix: remove SPRING_TEMPERATURE
4344a8a 
already managed by `weather_manager::update_weather`
@scarf005
refactor: use early return
e3dd2f3
@scarf005
fix: redundant comment
5bea945
@olanti-p olanti-p merged commit 0085ef6 into cataclysmbnteam:upload Sep 21, 2023
@scarf005 scarf005 deleted the remove-fahrenheit branch December 16, 2023 12:14
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 16, 2023
@scarf005 @Coolthulhu
refactor: explicitly use to_millidegree_celsius
a0f3019 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 16, 2023
@scarf005 @Coolthulhu
refactor: simplify celsisus to fahrenheit calls
b278cb8 
discussed in
- cataclysmbnteam#2570 (comment)
- cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 16, 2023
@scarf005 @Coolthulhu
refactor: use milidegree_celsius
34cd7ef 
discussed in: cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 16, 2023
@scarf005 @Coolthulhu
refactor: make arbitrary temperatures look arbitrary
565893f 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
@scarf005 scarf005 mentioned this pull request Dec 16, 2023
Merged
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 17, 2023
@scarf005 @Coolthulhu
refactor: explicitly use to_millidegree_celsius
5be3b16 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 17, 2023
@scarf005 @Coolthulhu
refactor: simplify celsisus to fahrenheit calls
9594e8e 
discussed in
- cataclysmbnteam#2570 (comment)
- cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 17, 2023
@scarf005 @Coolthulhu
refactor: use milidegree_celsius
20477d2 
discussed in: cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Dec 17, 2023
@scarf005 @Coolthulhu
refactor: make arbitrary temperatures look arbitrary
20209b2 
discussed in cataclysmbnteam#2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>
scarf005 added a commit that referenced this pull request Dec 18, 2023
@scarf005 @Coolthulhu
refactor: fahrenheit to celsius, part 2 (#3926)
4b5f9ba 
* refactor: `water_temperature` to `units::temperature`

* refactor: `weather_manager::temperatrue` to `units::temperature`

* refactor: explicitly use `to_millidegree_celsius`

discussed in #2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>

* refactor: simplify celsisus to fahrenheit calls

discussed in
- #2570 (comment)
- #2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>

* refactor: use `milidegree_celsius`

discussed in: #2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>

* refactor: make arbitrary temperatures look arbitrary

discussed in #2570 (comment)

Co-authored-by: Coolthulhu <[email protected]>

* test: adjust lossy conversion from fahrenheit to celsius

* refactor: remove `freezing_point`

it was never used for years.

* fix: round temperature manually

* refactor: use `units::temperature` for `weather_manager`

---------

Co-authored-by: Coolthulhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@olanti-p olanti-p olanti-p approved these changes

@Coolthulhu Coolthulhu Awaiting requested review from Coolthulhu

Assignees
No one assigned
Labels
src changes related to source code. tests changes related to tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

change temperature in codebase from fahrenheit to celcius
5 participants
@scarf005 @TheEaterOfBeans @chaosvolt @Coolthulhu @olanti-p